bpo-33014: Clarify str.isidentifier docstring#6088
Conversation
There was a problem hiding this comment.
To test whether string s is a reserved identifier you have to call keyword.iskeyword(s), not keyword.iskeyword().
I don't think anything should be changed here. Currently the link removes even the smallest chance of misunderstanding it. IMHO changes will make the documentation worse.
There was a problem hiding this comment.
In my suggestion on the issue, I specifically included the argument '(s)' in the call so that 's' later in the sentence referred back to the string passed as an argument. I changes the vague 'use' to the specific 'call'. 'test for reserved identifiers' is awkward. Test what? (especially with the subject s never mentioned). Changes are not always bad.
I like the following, based on Serhiy's comment, even better as a followup to the preceding sentence.
To test whether string s is a reserved keyword, such as def or class, call keyword.iskeyword(s).
There was a problem hiding this comment.
Sure, I will change to that. The only issue I faced adding an argument in the function is that it is not hyperlinked in the html docs if I did :func:keyword.iskeyword(s). Although :func:keyword.iskeyword works fine.
I searched the docs for a similar function reference that accepts an argument but I couldn't find. Can you please guide me on how do I change it so that it is also hyperlinked to the actual method definition in the html docs?
There was a problem hiding this comment.
This is the reason why I suggest to not change the current documentation. It is good enough and changes make it worse: technically incorrect, more verbose, or less informative.
There was a problem hiding this comment.
CuriousLearner: I presume, but don't know for sure, that you can pur '(x)' outside the backticks and get the hyperlink.
Serhiy: Either proposed change, the second of which is based on your statement, is more informative, not less, and more technically correct, not less. Your said it yourself. iskeyword "test[s] whether string s is a reserved identifier [singular]", whereas the somewhat cryptic 'test for reserved identifiers [plural]' can easily be expanded to and construed as meaning 'test whether string s contains reserved identifiers [plural]". The IDLE colorizer does just that. It 'tests for reserved keywords', in this sense, and when it finds them, tags them.
There was a problem hiding this comment.
@terryjreedy I already tried that & it does not get hyperlinked correctly.
@Mariatta Can you help here please?
1c73ffd to
d418e9f
Compare
d418e9f to
1cb2300
Compare
willingc
left a comment
There was a problem hiding this comment.
@CuriousLearner Thanks. I believe adding the example will clarify usage.
| Use :func:`keyword.iskeyword` to test for reserved identifiers such as | ||
| :keyword:`def` and :keyword:`class`. | ||
| Call :func:`keyword.iskeyword` to test whether string ``s`` is a reserved | ||
| identifier, such as :keyword:`def` and :keyword:`class`. |
There was a problem hiding this comment.
Adding an example would clarify the usage and differences:
Keywords, which are a small subset of identifiers used as reserved words, are not ordinary identifiers. To test if a string is a keyword, call :func:keyword.iskeyword to test whether a string s is a reserved identifier, such as :keyword: def and :keyword:class.
An example shows their usage:
>>> import keyword
>>> string_ordinary = 'hello'
>>> string_keyword = 'def'
>>> string_ordinary.isidentifier()
True
>>> keyword.iskeyword(string_ordinary)
False
>>> string_keyword.isidentifier()
True
>>> keyword.iskeyword(string_keyword)
True
There was a problem hiding this comment.
This proposed change is in an entry about identifiers, not keywords. Expanding the keyword module doc would be a separate discussion. If we did something, I would it a bit shorter, such as
>>> from keyword import iskeyword
>>> 'hello'.isidentifier(), iskeyword('hello')
True, False
>>> 'def'.isidentifier(), iskeyword('def')
True, True
There was a problem hiding this comment.
I like @terryjreedy’s suggested example. More compact but captures the importance of contrasting both uses.
|
Travis failed because it runs a sphinx 'make suspicious' check which did not like something in the new markup. I don't know why. |
|
@Mariatta Any updates on this one? |
|
@CuriousLearner I would go ahead and make @terryjreedy's suggested edit. Then push that change to this PR which hopefully will resolve the issue with the |
…ssue-33014 * 'master' of github.com:CuriousLearner/cpython: (722 commits) bpo-34087: Fix buffer overflow in int(s) and similar functions (pythonGH-8274) bpo-34108: Fix double carriage return in 2to3 on Windows (python#8271) bpo-4260: Document that ctypes.xFUNCTYPE are decorators (pythonGH-7924) bpo-33723: Fix test_time.test_thread_time() (pythonGH-8267) bpo-33967: Remove use of deprecated assertRaisesRegexp() (pythonGH-8261) bpo-34080: Fix a memory leak in the compiler. (pythonGH-8222) Enable GUI testing on Travis Linux builds via Xvfb (pythonGH-7887) bpo-23927: Make getargs.c skipitem() skipping 'w*'. (pythonGH-8192) bpo-33648: Remove PY_WARN_ON_C_LOCALE (pythonGH-7114) bpo-34092, test_logging: increase SMTPHandlerTest timeout (pythonGH-8245) Simplify __all__ in multiprocessing (pythonGH-6856) bpo-34083: Update dict order in Functional HOWTO (pythonGH-8230) Doc: Point to Simple statements section instead of PEP (pythonGH-8238) bpo-29442: Replace optparse with argparse in setup.py (pythonGH-139) bpo-33597: Add What's New for PyGC_Head (pythonGH-8236) Dataclasses: Fix example on 30.6.8, add method should receive a list rather than an integer. (pythonGH-8038) Fix documentation for input and output tutorial (pythonGH-8231) bpo-34009: Expand on platform support changes (pythonGH-8022) Factor-out two substantially identical code blocks. (pythonGH-8219) bpo-34031: fix incorrect usage of self.fail in two tests (pythonGH-8091) ...
|
Sure, thanks @willingc I have made the requested changes; please review again |
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
|
Hey, @terryjreedy @willingc @serhiy-storchaka I've made the changes. Can you please have a look now? |
|
@willingc please review |
|
Carol, I presume your comment is a note to yourself. My approval is of the content, not the markup, which I leave to you. |
|
@terryjreedy Yes, I met Carol today in PyCon India and asked if she would have some time to look at the patches that are waiting in the queue, so she added a note for herself to remember reviewing them later :) |
|
Thanks @CuriousLearner for your patience and contribution. |
|
Thank you so much @willingc for merging this :) It was an absolute pleasure meeting with you in person :) |
Currently, I've updated the docs for
str.isidentifierclarifying the usage ofkeyword.iskeywordFor updating the docstring of
keyword.iskeyword, I saw thatLib/Keyword.pydefines this on line 55:iskeyword = frozenset(kwlist).__contains__The docstring of the file says that it is automatically generated from
graminit.c. I observed that file and have no clue on how to proceed. Can someone provide me a pointer on this please?https://bugs.python.org/issue33014